ARROW-14815 [R] bindings for `lubridate::semester()` by dragosmg · Pull Request #12429 · apache/arrow

dragosmg · 2022-02-15T11:16:27Z

Once this PR is merged the following code snippet will be valid

suppressPackageStartupMessages(library(dplyr))
suppressPackageStartupMessages(library(lubridate))
suppressPackageStartupMessages(library(arrow))

df <- tibble(
  dates = as.Date(c("2021-04-30", "2021-07-31", "2021-10-31", "2021-01-31"))
  )
df %>% 
  record_batch() %>% 
  mutate(
    semester_without_year = semester(dates),
    semester_with_year = semester(dates, with_year = TRUE)) %>% 
  collect()
#> # A tibble: 4 × 3
#>   dates      semester_without_year semester_with_year
#>   <date>                     <int>              <dbl>
#> 1 2021-04-30                     1              2021.1
#> 2 2021-07-31                     2              2021.2
#> 3 2021-10-31                     2              2021.2
#> 4 2021-01-31                     1              2021.1

and identical with the lubridate result:

suppressPackageStartupMessages(library(dplyr))
suppressPackageStartupMessages(library(lubridate))

df <- tibble(
  dates = as.Date(c("2021-04-30", "2021-07-31", "2021-10-31", "2021-01-31"))
  )
df %>% 
  mutate(
    semester_without_year = semester(dates),
    semester_with_year = semester(dates, with_year = TRUE))
#> # A tibble: 4 × 3
#>   dates      semester_without_year semester_with_year
#>   <date>                     <int>              <dbl>
#> 1 2021-04-30                     1              2021.1
#> 2 2021-07-31                     2              2021.2
#> 3 2021-10-31                     2              2021.2
#> 4 2021-01-31                     1              2021.1

^{Created on 2022-02-15 by the reprex package (v2.0.1)}

github-actions · 2022-02-15T11:16:47Z

https://issues.apache.org/jira/browse/ARROW-14815

github-actions · 2022-02-15T11:16:49Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

jonkeane

This is looking good, thanks!

One comment (TIL about the return value of lubridate::semseter()!), and a request for some more testing on various kinds of (bad) inputs

r/R/dplyr-funcs-datetime.R

r/tests/testthat/test-dplyr-funcs-datetime.R

dragosmg · 2022-02-16T11:13:57Z

Not sure if we should (and if yes, at what level) support integer / numeric arguments to month(). If the answer is yes, I feel that should not happen only inside / for semester. lubridate::month() has dual purpose - it can be used both to get and set months. arrow currently supports only extraction and, therefore, the input type needs to be temporal.

jonkeane

This is getting closer — a few small changes.

It's up to you if you want to sweep the r-based month() improvements in to this ticket or do those improvements + the changes you'll need to these tests later as a separate ticket soon after, but we should do those sooner rather than later.

r/tests/testthat/test-dplyr-funcs-datetime.R

jonkeane

Thanks for pulling in some of the month changes, that's looking like it's headed in the right direction.

I've got a few comments + a few requests for a few more tests. Also a longer point (which you might already be totally aware of!) about how we might test bindings like this where the underlying data changes has an impact on what we need to do.

r/tests/testthat/test-dplyr-funcs-datetime.R

r/R/dplyr-funcs-datetime.R

jonkeane

Looks good for what we can do before we expand month() could you resolve the conflicts and then I'll merge?

* the logic is correct, but the implementation fails

ursabot · 2022-02-22T20:11:31Z

Benchmark runs are scheduled for baseline = 5680d20 and contender = 16f36a5. 16f36a5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.46% ⬆️0.04%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.34% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

dragosmg marked this pull request as ready for review February 15, 2022 11:16

github-actions bot added the Component: R label Feb 15, 2022

jonkeane requested changes Feb 15, 2022

View reviewed changes

r/R/dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

r/tests/testthat/test-dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

r/tests/testthat/test-dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

jonkeane requested changes Feb 16, 2022

View reviewed changes

r/tests/testthat/test-dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

r/tests/testthat/test-dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

r/tests/testthat/test-dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

dragosmg requested a review from jonkeane February 17, 2022 16:30

jonkeane requested changes Feb 19, 2022

View reviewed changes

r/tests/testthat/test-dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

r/R/dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

r/R/dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

r/R/dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

dragosmg requested a review from jonkeane February 21, 2022 16:22

jonkeane approved these changes Feb 22, 2022

View reviewed changes

dragosmg added 11 commits February 22, 2022 14:16

first pass at semester() + unit tests

ad67858

added NEWS bullet point

25460cc

typo

583d515

reorganise unit tests and add reference to ARROW-15701

77c3298

a bit more specific about the error message

522e2b2

use sprintf() and add TODO

2aab652

early pass at supporting integer inputs for month()

eff83bd

* the logic is correct, but the implementation fails

simplified the unit testing blocks

7a196c8

supporting R integers & arrow expressions

01e0b3c

remove support for integer months and delegate that to ARROW-15701

4e64268

solve merge conflict weirdness

0e334d1

dragosmg force-pushed the lubridate_semester branch from 7e0a2af to 0e334d1 Compare February 22, 2022 14:20

polished NEWS.md

c1d96f0

jonkeane closed this in 16f36a5 Feb 22, 2022

dragosmg deleted the lubridate_semester branch February 23, 2022 09:44

asfimport mentioned this pull request Feb 23, 2022

[R] Implement bindings for lubridate::semester #30348

Closed

Conversation

dragosmg commented Feb 15, 2022

Uh oh!

github-actions bot commented Feb 15, 2022

Uh oh!

github-actions bot commented Feb 15, 2022

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dragosmg commented Feb 16, 2022

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonkeane left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ursabot commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonkeane left a comment •

edited

Loading

ursabot commented Feb 22, 2022 •

edited

Loading